-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[typescript] Constrain props type param appropriately in withStyles, withTheme, withWidth HOCs #11003
Conversation
My experience is that the current `withStyles` declaration works poorly with Typescript 2.8. I'm not sure that it's the compiler version that's the issue, though. At least in my usage, it fails to remove `WithStyles<ClassKey>` from the return type. This definition works better in my experience, doesn't rely on subtle inferencer behavior in the same way, and seems to be in the mainstream of current usage. There's a built-in `Exclude` function in Typescript 2.8 that does what `Omit` does. I used `Omit` for compatibility with older compilers.
@pelotom Tom, if you've a few cycles, could you look at this before I try to fix all the downstream issues? You may have tried this approach and didn't get good results, and/or may have concerns about breaking changes (ahem ;-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many variations on this signature have been tried and abandoned because of one pain point or another. If you fix the tests we can see what it looks like and evaluate whether it's better on balance.
src/styles/withStyles.d.ts
Outdated
@@ -38,9 +38,13 @@ export interface StyledComponentProps<ClassKey extends string = string> { | |||
innerRef?: React.Ref<any>; | |||
} | |||
|
|||
// Diff / Omit taken from https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-311923766 | |||
export type Diff<T extends string, U extends string> = ({ [P in T]: P } & { [P in U]: never } & { [x: string]: never })[T]; | |||
export type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, Diff
and Omit
are already defined in index.d.ts
:
src/styles/withStyles.d.ts
Outdated
style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>, | ||
options?: WithStylesOptions, | ||
): <P extends WithStyles<ClassKey>>( | ||
component: React.ComponentType<P>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous indentation (use yarn prettier
to clean this up).
So in all use cases within the test code you have increased the notational burden with this change. How is it supposed to be better again? |
I'll write up an issue with examples and link this to it. In my own usage, I create and expect a type definition at hand for the entire set of props that a component expects, including those injected by HOCs, so the "increased notational burden" doesn't exist in my small universe. But of course, other folks' usage Some other HOC declarations, such as those for react-router: export function withRouter<P extends RouteComponentProps<any>>(component: React.ComponentType<P>): React.ComponentClass<Omit<P, keyof RouteComponentProps<any>>>; require such a type as a generic input. If the component is well-typed, the generic is inferred. So in the current case, this would work: const Decoratee: React.SFC<Props & WithStyles<'root'>> = ({ text, variant, color, classes }) => (
<Typography variant={variant} color={color} classes={classes}>
{text}
</Typography>
)
const DecoratedSFC = decorate(Decoratee) If this PR is otherwise a good idea (which needs to be evaluated in the context of the issue-to-be-written), I should probably update the tests to reflect this style of writing. |
@pelotom I won't be submitting an issue after all; I just figured out the nature and source of the issue. See the updated initial comment, above. |
That PR wasn't merged, so how could it be "causing" anything? |
Oy - yeah. I'm not sure what made this become evident. The rest stands, though, IMHO. Obviously, it's a breaking change. For folks who use typed components, it means deleting the generic parameter, which will probably be welcomed. For others, not so much! |
Sorry, I'm still not seeing the benefit. Please provide a complete, self-contained example showing what it looks like with the current typing and how your proposed typing improves it. |
I've updated the tests so that they make the case; if they don't float your boat, probably nothing will, and it's time to close this. At the risk of belaboring the obvious: declaring the properties and their types of a component has independent value in helping in IDE and compiler catch errors that otherwise will only be detected at runtime. Dragging people into doing this may cause a little grumbling, but I think it'll be less than the grumbling from needlessly requiring a generic parameter. I started this thinking I was seeing a bug; I just assumed that one would design these HOC declarations so that they would work well with inferred typing. I can't begin to guess how common that expectation is; it may be just me. |
@oliviertassinari fwiw, Argos appears to be stuck. No urgency here! |
@estaub Yeah, Argos-CI is down. I have a weird out of memory issue. |
@pelotom When you get to reviewing this, I'd suggest that you look at the full-text (not just diff) of styles.spec.ts. The diff makes it hard to see the gist of it. |
@estaub Argos-CI is back to normal ✅. |
attempting to get tests to rerun... |
src/styles/withStyles.d.ts
Outdated
) => React.ComponentType<P & StyledComponentProps<ClassKey>>; | ||
): <P extends WithStyles<ClassKey>>( | ||
component: React.ComponentType<P>, | ||
) => React.ComponentType<Omit<P, 'classes'> & StyledComponentProps<ClassKey>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some merit here, but P
should not be required to included a classes
prop. Instead it should be required that if P
does include a classes
prop, it should have the right type. Something like this:
interface MaybeWithStyles extends Partial<WithStyles<ClassKey>> {
[k: string]: any;
}
export default function withStyles<ClassKey extends string>(
style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
options?: WithStylesOptions,
): <P extends MaybeWithStyles<ClassKey>>(
component: React.ComponentType<P & WithStyles<ClassKey>>,
) => React.ComponentType<Omit<P, 'classes'> & StyledComponentProps<ClassKey>>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks great! Let me check it out a bit.
That commit went well - the only changes in the tests, from the original, are now the (optional) removals of generic parameters. |
src/utils/withWidth.d.ts
Outdated
) => React.ComponentClass<P & Partial<WithWidthProps>>; | ||
): <P extends WithWidthProps>( | ||
component: React.ComponentType<P>, | ||
) => React.ComponentClass<Omit<P, keyof WithWidthProps> & Partial<WithWidthProps>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, this shouldn't require P
to contain width
:
export default function withWidth(
options?: WithWidthOptions,
): <P extends Partial<WithWidthProps> & Record<string, any>>(
component: React.ComponentType<P & WithWidthProps>,
) => React.ComponentClass<Omit<P, keyof WithWidthProps> & Partial<WithWidthProps>>;
With all of these HOCs we want to enable this kind of pattern:
const Component = withWidth()<{
// shouldn't need to specify width here; it's a given
name: string
}>(({ width, name }) =>
<div style={{ width }}>hello, {name}</div>
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wondered about those two, should have hit them. I'll add a couple of tests, too.
src/styles/withTheme.d.ts
Outdated
) => React.ComponentClass<P>; | ||
declare const withTheme: () => <P extends WithTheme>( | ||
component: React.ComponentType<P>, | ||
) => React.ComponentClass<Omit<P, keyof WithTheme>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, this shouldn't require P
to contain theme
...
I'm pretty sure this means both of the "2 scenarios" mentioned in https://material-ui.com/guides/typescript/ about having to specify explicit type parameters can be removed now. It would be good to do that as part of this PR. |
src/styles/withStyles.d.ts
Outdated
): <P extends WithStyles<ClassKey>>( | ||
component: React.ComponentType<P>, | ||
) => React.ComponentType<Omit<P, 'classes'> & StyledComponentProps<ClassKey>>; | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get rid of this instead of commenting out.
src/styles/withStyles.d.ts
Outdated
|
||
interface MaybeWithStyles<ClassKey extends string> extends Partial<WithStyles<ClassKey>> { | ||
[k: string]: any; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making a reusable version of this,
type ConsistentWith<O> = Partial<O> & Record<string, any>;
which can then be used for withStyles
, withWidth
and withTheme
:
P extends ConsistentWith<WithStyles<ClassKey>>
P extends ConsistentWith<WithWidthProps>
P extends ConsistentWith<WithTheme>
Remove commented-on code from withStyles; use ConsistentWith typewrapper; add SFC test for withWidth add FAILING test for doc scenario 2 (withStyles of union)
Surprisingly at this point, I've reached a hard blocker with this. The "DecoratedUnionProps" or "Scenario 2" example on https://material-ui.com/guides/typescript/ didn't have a test, so I threw one into The "Omit" implementation fails to retain the alternative properties in a union. In the example, for "ArtProps" it retains"category" but not "artist" or "author". I didn't find any way that providing the current generic parameter explicitly helps, and doubt there would be any. If you have any ideas, I'm all ears twice over! I did some experimenting with the new 2.8 Exclude, and it also fails, with a different message, complaining that the union is not an interface. Here are the options I can see:
I didn't check in any doc changes, pending where we go. |
I don't know what's going on, but the ci behavior was very strange too, first failing, then succeeding. So chances are others would get the behavior you're seeing. I rolled back that test change and doc change; AFAIK, this PR is ready to merge. Tangentially... where did you learn the multi-signature syntax you used to fix |
I think you're confusing yourself somehow. The union type argument has always been there in the test, you never removed it. Your last commit says
but if you look at the actual diff nothing changed in the test.
It's called "overloading", and it's been around since the very early days of TypeScript. All I can say is, you will learn over time with practice. I wish they still maintained an up-to-date spec, but they don't 🤷♂️ |
package.json
Outdated
@@ -46,7 +46,7 @@ | |||
"test:coverage:html": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha test/**/*.test.js src/{,**/}*.test.js && nyc report --reporter=html", | |||
"test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js --single-run", | |||
"test:regressions": "webpack --config test/regressions/webpack.config.js && rimraf test/regressions/screenshots/chrome/* && vrtest run --config test/vrtest.config.js --record", | |||
"typescript": "tsc -p tsconfig.json", | |||
"typescript": "tsc -v && tsc -p tsconfig.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
package.json
Outdated
@@ -187,7 +187,7 @@ | |||
"rimraf": "^2.6.2", | |||
"sinon": "^4.1.2", | |||
"size-limit": "0.14.1", | |||
"typescript": "^2.6.1", | |||
"typescript": "2.6.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but again, it doesn't compile in 2.6.1. Do you want me to remove the fragment from the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't matter, the yarn.lock
is what controls the actual version this project depends on, and it did point to 2.8.1
until you changed it. There should be no changes to either package.json
or yarn.lock
in this PR.
@@ -68,11 +68,11 @@ const DecoratedUnionProps = decorate<ArtProps>( // <-- without the type argument | |||
const props = this.props; | |||
return ( | |||
<Typography classes={props.classes}> | |||
{props.category === 'book' ? props.author : props.artist} | |||
{props.category === "book" ? props.author : props.artist} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done, but what is the intended preference?
Both are used in that file, roughly 50/50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My immediate preference is to keep irrelevant changes out of this PR 🙂
</Typography> | ||
); | ||
} | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
yarn.lock
Outdated
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.8.1.tgz#6160e4f8f195d5ba81d4876f9c0cc1fbc0820624" | ||
typescript@2.6.2: | ||
version "2.6.2" | ||
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.6.2.tgz#3c5b6fd7f6de0914269027f03c0946758f7673a4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -8,7 +8,7 @@ Have a look at the [Create React App with TypeScript](https://github.com/mui-org | |||
The usage of `withStyles` in TypeScript can be a little tricky, so it's worth showing some examples. You can first call `withStyles()` to create a decorator function, like so: | |||
|
|||
```js | |||
const decorate = withStyles(({ palette, spacing }) => ({ | |||
const withMyStyles = withStyles(({ palette, spacing }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't rename decorate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Use single quotes in test. In package.json, revert typescript to 2.6.1, remove version-printing run command In package.lock, revert typescript to 2.6.1.
What do you want me to on the merge? |
|
||
Scenario 2: `Props` is a union type. Again, to avoid getting a compiler error, you'll need to provide an explict type argument: | ||
When your `props` are a union, Typescript needs you to explicitly tell it the type, | ||
by providing an explicit generic `<Props>` parameter to `decorate`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
package.json
Outdated
@@ -187,7 +187,7 @@ | |||
"rimraf": "^2.6.2", | |||
"sinon": "^4.1.2", | |||
"size-limit": "0.14.1", | |||
"typescript": "^2.6.1", | |||
"typescript": "2.6.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be reverted.
There are some conflicts with |
I'm not sure what you're asking, but you need to merge |
src/styles/withStyles.d.ts
Outdated
@@ -1,4 +1,6 @@ | |||
import * as React from 'react'; | |||
import { WithTheme } from '../styles/withTheme'; | |||
import { Omit, ConsistentWith } from '..'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Omit
import is no longer necessary so it should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/styles/withStyles.d.ts
Outdated
<P extends ConsistentWith<WithStyles<ClassKey>>>( | ||
component: React.ComponentType<P & WithStyles<ClassKey>>, | ||
): React.ComponentType<P & StyledComponentProps<ClassKey>>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon
``` | ||
|
||
Scenario 2: `Props` is a union type. Again, to avoid getting a compiler error, you'll need to provide an explict type argument: | ||
When your `props` are a union, Typescript needs you to explicitly tell it the type, by providing an explicit generic `<Props>` parameter to `decorate`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-by providing an explicit generic
+by providing a generic
Remove the "explicit", it's implicit that it's explicit from the preceding "explicitly" :)
@@ -46,3 +46,33 @@ const DecoratedNoProps = decorate<{}>( | |||
const sfcElem = <DecoratedSFC text="Hello, World!" variant="title" color="secondary" />; | |||
const classElem = <DecoratedClass text="Hello, World!" variant="title" color="secondary" />; | |||
const noPropsElem = <DecoratedNoProps />; | |||
|
|||
// This is the "scenario 2" example straight from the doc, then invoked: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no scenario 2 now, maybe just provide a link or else delete the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks, it was a long slog but worth it! |
Back at you! |
@pelotom I didn't want to spam the subscribers on # 11109 with implementation discussion, so this is here. The earlier contravariance writeup got me thinking backward about a conditional typing approach to this, and you may well be right that there's an answer there. This 2.8-dependent (WithStyles<ClassKey> extends P? P: never) export default function withStyles<ClassKey extends string>(
style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
options?: WithStylesOptions,
): {
<P extends WithStyles<ClassKey>>(
component: React.ComponentType<P & (WithStyles<ClassKey> extends P? P: never)>):
React.ComponentType<{_1?:never} & StyledComponentProps<ClassKey>>;
<P extends ConsistentWith<WithStyles<ClassKey>>>(
component: React.ComponentType<P & WithStyles<ClassKey>>,
): React.ComponentType<{_2?:never} & P & StyledComponentProps<ClassKey>>;
}; I'm afraid I'm going to have to bail on this now, or at least very soon; it's been a few orders of magnitude more time than I ever imagined. If you come to think a rollback is in order, just do it. |
still reports error for #11191 |
UPDATED
The current structure of withStyles, withTheme, and withWidth forces use of a generic parameter to describe the component's properties without the injected properties.
This changes the generic parameter to reflect the entire props of the inner component, so that it can be inferred from the passed component.